Fix lookup-table reaper deleting rows referenced by concurrent ingestion - #222
Merged
Conversation
The lookup-table reaper could delete a history lookup row (history_accounts, history_assets, history_claimable_balances, history_liquidity_pools) that live ingestion was concurrently inserting a reference to, leaving a dangling reference in the history tables (history_operation_participants, history_effects, history_trades, ...). At the API level this surfaces as a 404 on an account's history endpoints, or a 200 that silently omits earlier records once the address is re-inserted with a new id. The reaper's DELETE combined the FOR UPDATE row lock and the NOT EXISTS orphan check in a single statement. Under READ COMMITTED the statement's snapshot is taken before it blocks on the lock, so once the ingestion transaction it blocked on committed, the NOT EXISTS sub-queries still ran against the pre-commit snapshot, did not see the newly inserted references, and the row was deleted while in use. The FOR UPDATE lock made the reaper block but did not refresh the snapshot used by the check. Split the lock and the orphan check into two separate statements in the reaper transaction: SELECT ... FOR UPDATE first, then DELETE ... WHERE NOT EXISTS. Because the DELETE is a separate statement it runs under a fresh snapshot taken after the lock wait resolves, so it observes the references committed by the ingestion transaction the reaper blocked on. Add a regression test that inserts a referencing child row while ingestion holds the FOR KEY SHARE lock and asserts the reaper leaves the row in place. This addresses concurrency with live ingestion. Reaping concurrently with `db reingest range` (which does not take the FOR KEY SHARE lock on the parent) is a separate follow-up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a race between live ingestion and lookup-table reaping that could create dangling history references.
Changes:
- Separates row locking from orphan deletion to obtain a fresh PostgreSQL snapshot.
- Adds query-builder and concurrency regression tests.
- Documents the fix in the changelog.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
internal/db2/history/main.go |
Splits locking and deletion queries. |
internal/db2/history/main_test.go |
Updates query-builder tests. |
internal/db2/history/reap_concurrency_test.go |
Tests the concurrent ingestion race. |
CHANGELOG.md |
Records the resolved issue. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
tamirms
force-pushed
the
fix-reaper-lookup-table-snapshot-race
branch
from
August 14, 2026 18:57
6301531 to
fbada24
Compare
Shaptic
approved these changes
Aug 14, 2026
- Enforce that deleteLookupTableRows runs inside a transaction via q.GetTx(), matching the pattern in verify_lock.go, instead of only documenting it. The FOR UPDATE lock is meaningless outside a transaction. - Run the lock query with SelectRaw instead of ExecRaw with a manually set SelectQueryType, so the query type is clear at the call site. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
tamirms
enabled auto-merge (squash)
August 15, 2026 07:09
Shaptic
added a commit
that referenced
this pull request
Aug 17, 2026
The lookup-table reaper fix landed on main after the release branch was cut, so it filed itself under Unreleased. v28.0.0 is tagged on this PR's merge commit, which has main as a parent, so #222 ships in it — move the entry into the 28.0.0 Fixed section where readers will look for it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The lookup-table reaper could delete a history lookup row (
history_accounts,history_assets,history_claimable_balances,history_liquidity_pools) while live ingestion was concurrently inserting a reference to it, leaving a dangling reference in the history tables (history_operation_participants,history_effects,history_trades, …). The lookup row'sidis the join key for an account's history, so at the API level this surfaces as:GET /accounts/{address}→ 200 with the correct balance (the account looks healthy), butGET /accounts/{address}/payments(and/operations,/effects,/trades) → 404, because the join tohistory_accountsreturns no rows.Once the address transacts again, the loader re-inserts it with a new id, so those endpoints return 200 again but silently omit every earlier record. The operation rows themselves are never deleted — only the join key — so a reingest of the retained range recovers them.
Root cause
constructDeleteLookupTableRowsQuerybuilt the reaper's delete as a single statement that both took aFOR UPDATErow lock and ran theNOT EXISTSorphan check:The
FOR UPDATElock was intended to make the reaper block until any concurrent ingestion (which locks the same rowsFOR KEY SHARE) commits. It does block — but under the defaultREAD COMMITTEDisolation the statement's snapshot is fixed when the statement starts, before it blocks. When the ingestion transaction it blocked on commits, theNOT EXISTSsub-queries still evaluate against the pre-commit snapshot, don't see the just-inserted references, report the row as orphaned, and delete it. Blocking on the lock does not refresh the snapshot the check runs under.Fix
Split the lock and the orphan check into two separate statements within the reaper's transaction:
SELECT id FROM <table> WHERE id IN (...) ORDER BY id ASC FOR UPDATE— take the locks and wait out any in-flight ingestion.DELETE FROM <table> WHERE id IN (...) AND NOT EXISTS (... references ...)— a separate statement, so its snapshot is taken after the lock wait resolves and it observes the references committed by the ingestion transaction the reaper blocked on. TheFOR UPDATElocks from statement 1 are held for the rest of the transaction, so no new ingestion can insert a reference in between.Scope
This fixes concurrency with live ingestion, which is where the reaper runs concurrently by design. Reaping concurrently with
db reingest range(the non---forcepath, which usesConcurrentInsertsand does not take theFOR KEY SHARElock on the parent) is a separate, narrower window and is left as a follow-up.Tests
TestReapDoesNotDeleteConcurrentlyReferencedRows(new): drives the exact interleaving — ingestion locks the accountFOR KEY SHAREand inserts a referencinghistory_operation_participantsrow, uncommitted; the reaper blocks on the lock; ingestion commits; the reaper must leave the row in place. This test fails against the previous single-statement query (deletes the row) and passes with the fix.TestConstructDeleteLookupTableRowsQueryand addedTestConstructLockLookupTableRowsQueryfor the new query builders.🤖 Generated with Claude Code